Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds InvalidRegex TaintConfiguration Error type #743

Closed
wants to merge 1 commit into from

Conversation

abishekvashok
Copy link
Contributor

@abishekvashok abishekvashok commented Jun 8, 2023

Pre-submission checklist

  • I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
  • pre-commit run

Summary

Adds new error type that can handle Re2 compilation failed exceptions. Previously, when a regex compilation failed, the exception wasn't caught and the program terminated abnormally.

Catch the exception, and throw a custom TaintConfiguration Error to expose the underlying reason why the compilation failed and exit in a more user-friendly fashion.

Test Plan

  • taint.config used:
{
  "sources": [
    {
      "name": "CustomUserControlled",
      "comment": "use to annotate user input"
    }
  ],
  "implicit_sources": {
     "literal_strings": [
       {
         "regexp": "\\d{1Z,3}(\\.\\d{1,3}+",
         "kind": "CustomUserControlled",
         "description": "String that looks like an IP address."
       }
     ]
  },
  "sinks": [
    {
      "name": "CodeExecution",
      "comment": "use to annotate execution of python code"
    }
  ],

  "features": [],

  "rules": [
    {
      "name": "Possible RCE:",
      "code": 5001,
      "sources": [ "CustomUserControlled" ],
      "sinks": [ "CodeExecution" ],
      "message_format": "User specified data may reach a code execution sink"
    }
  ]
}

Before:
before

After:
after

Fixes part of: MLH-Fellowship#82
Signed-off-by: Abishek V Ashok abishekvashok@gmail.com

  • make test

@abishekvashok
Copy link
Contributor Author

Footnotes:

  • Pysa Github Actions were failing before and is unrelated to this PR.
  • This might cause conflicts with Display positions in SourceDuplicate errors #741 as both define the same json_string_member_with_location function -> but not sure. When either of these is merged, I will rebase the other.

@connernilsen
Copy link
Contributor

Hey @abishekvashok, I'm sorry, this PR slipped by me and I just noticed. Let me forward it to the Pysa team for you.

@abishekvashok
Copy link
Contributor Author

@connernilsen thank you. There's also #739 and #741 if you don't mind 😁 Also do take your time :)

Copy link
Contributor

@arthaud arthaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 916 to 935
let json_string_member_with_location ~path key value =
json_exception_to_error ~path ~section:key (fun () ->
let node = JsonAst.Json.Util.member_exn key value in
let value = JsonAst.Json.Util.to_string_exn node in
let location = JsonAst.Json.Util.to_location_exn node in
Result.return (value, location))
in
Copy link
Contributor

@arthaud arthaud Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like #741 also adds this, so we will need to be careful when merging this.
EDIT: Just noticed this was already mentioned above..

@facebook-github-bot
Copy link
Contributor

@arthaud has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Adds new error type that can handle Re2 compilation failed exceptions.
Previously, when a regex compilation failed, the exception wasn't caught
and the program terminated abnormally.

Catch the exception, and throw a custom TaintConfiguration Error to
expose the underlying reason why the compilation failed and exit in a
more user-friendly fashion.

Signed-off-by: Abishek V Ashok <abishekvashok@gmail.com>
@abishekvashok
Copy link
Contributor Author

@arthaud rebased on top of main :) You may want to re-import.

@facebook-github-bot
Copy link
Contributor

@arthaud merged this pull request in 7dee807.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants